Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ct 716 add grants to materializations #178

Merged
merged 28 commits into from
Jul 11, 2022
Merged

Conversation

McKnight-42
Copy link
Contributor

@McKnight-42 McKnight-42 commented Jun 27, 2022

resolves: CT-716
related to: CT-660

Before merge

  • Reinstate original version of dev-requirements.txt

Description

implementing apply_grants, will add tests, for snowflake instances.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-snowflake next" section.

@cla-bot cla-bot bot added the cla:yes label Jun 27, 2022
@McKnight-42 McKnight-42 self-assigned this Jun 27, 2022
@McKnight-42
Copy link
Contributor Author

7/5/22 EOD REPORT: main report can be found on SQL GRANTS. did have a question on if the OWNERSHIP check here was unneeded

@McKnight-42 McKnight-42 marked this pull request as ready for review July 5, 2022 22:16
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!! This is running for me locally.

SHOULD fix (inefficient but correct):

  • Let's figure out the right approach to should_revoke on Snowflake. I left a big comment talking through this. It will have implications for the implementation we ultimately land on in dbt-core.

for row in grants_table:
grantee = row['grantee_name']
privilege = row['privilege']
if privilege != 'OWNERSHIP':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OWNERSHIP filter makes sense to me, and is one valid way of proceeding here.

My inclination would be to generalize this beyond just OWNERSHIP: all grants given BY the current user (=role), and NOT TO the current user (=role):

for row in grants_table:

    # three relevant columns
    grantee = row['grantee_name']
    privilege = row['privilege']
    granted_by = row['granted_by']

    # super gross, but it works
    # (this is the value from profiles.yml)
    current_role = self.connections.profile.credentials.role
    
    if (
        # granted BY the current role
        granted_by.lower() == current_role.lower() and
        
        # NOT granted TO the current role
        grantee.lower() != current_role.lower()
    ):
        ...


{% set target_relation = this.incorporate(type='view') %}

{% do apply_grants(target_relation, grant_config, should_revoke=False) %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this here if apply_grants is already happening within create_or_replace_view() (here)

@@ -25,6 +27,8 @@

{{ run_hooks(post_hooks) }}

{% do apply_grants(target_relation, grant_config, should_revoke=True) %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's talk about should_revoke!

View + table:

  • should_revoke = False, because grants are NOT carried over
  • UNLESS the user has explicitly opted into the copy_grants config, in which case they are
  • What should our behavior be? I think it should be conditional based on copy_grants, and this could simply be should_revoke = copy_grants. Either both are False or both are True.

Incremental, seed, snapshot: These have the added questions of: Are we building this table for the first time? Are we fully replacing (full-refreshing) it? If we ARE fully replacing it, we should use the exact same conditional logic (based on copy_grants) to determine

If we are going to have conditional logic based on copy_grants, it requires us to have different logic for different models, rather than a single True/False setting for the entire adapter. That's a vote in favor of putting this logic into the materialization, rather than an adapter method, as discussed in dbt-labs/dbt-core#5369 (comment)

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 6, 2022

@McKnight-42 @dbeatty10 Follow-ups from pairing work: ct-716-add-materialization...jerco/grants-over-finish-line

More context in dbt-labs/dbt-core#5369 (comment). Implements most of the SHOULD + COULD comments from above.

@McKnight-42 McKnight-42 requested a review from jtcohen6 July 6, 2022 20:41
@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 7, 2022

@McKnight-42 Functional/integration tests are failing because this branch depends on the changes in dbt-labs/dbt-core#5369. Could you update these lines:

git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-core&subdirectory=core
git+https://github.com/dbt-labs/dbt-core.git#egg=dbt-tests-adapter&subdirectory=tests/adapter

To point to your dbt-core branch (ct-660-grant-sql:

git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-core@&subdirectory=core
git+https://github.com/dbt-labs/dbt-core.git@ct-660-grant-sql#egg=dbt-tests-adapter&subdirectory=tests/adapter

This is exactly how @dbeatty10 and I were doing the lift-and-shift of cross-database macros. Once the dbt-core PR is merged, we can repoint to the main branch. But it's important to know ahead of time that the functionality will work in all our adapters.

@McKnight-42 McKnight-42 closed this Jul 7, 2022
@McKnight-42 McKnight-42 reopened this Jul 7, 2022
Copy link
Contributor

@dbeatty10 dbeatty10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @McKnight-42 !

@McKnight-42 McKnight-42 merged commit d53c327 into main Jul 11, 2022
@McKnight-42 McKnight-42 deleted the ct-716-add-materialization branch July 11, 2022 20:30
@dataders dataders changed the title Ct 716 add materialization Ct 716 add grants to materializations Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-716] Snowflake: Add Grants to Materializations
3 participants